Skip to content

fix(config): distinguish disabled-plugin toolsets from typos in validation - #56731

Closed
dante32683 wants to merge 2 commits into
NousResearch:mainfrom
dante32683:fix/toolset-cross-list-validation
Closed

dante32683 wants to merge 2 commits into
NousResearch:mainfrom
dante32683:fix/toolset-cross-list-validation

Conversation

@dante32683

Copy link
Copy Markdown

What changed

validate_platform_toolsets() (hermes_cli/toolset_validation.py, added for #38798) now accepts an optional third argument, known_plugin_toolsets, and cross-references it against unknown toolset names to produce a more accurate warning when the real cause is a disabled/uninstalled plugin rather than a typo.

Why

Hermes tracks toolset names across three separate places:

  • plugins.enabled — which plugins are turned on
  • known_plugin_toolsets — per-platform snapshot of plugin toolset keys seen the last time hermes tools saved that platform (hermes_cli/tools_config.py::_save_platform_tools)
  • platform_toolsets — the actual per-platform toolset selection

validate_platform_toolsets already does excellent work catching corrupted/renamed entries (the #38798 incident: hermes-cli silently rewritten to the nonexistent hermes), including a hermes-<platform> "did you mean" guess. But that guess is actively misleading for a very common real-world case: a platform_toolsets entry that names a real plugin toolset whose plugin is now disabled (or its package uninstalled). Since is_valid_toolset (backed by the live toolset registry) only knows about currently-loaded plugins, a disabled plugin's toolset name looks identical to a typo to the existing check — same generic "unknown toolset ''" warning, with no hermes-<platform> hint either (since that guess doesn't happen to match), leaving the real cause unstated.

known_plugin_toolsets already has exactly the information needed to tell these apart: if a name is in known_plugin_toolsets[platform], it was a real, valid plugin toolset the last time hermes tools ran for that platform — so its current invalidity almost certainly means the plugin got disabled or uninstalled, not that the name was ever wrong.

Approach

  • New optional known_plugin_toolsets param, default None (or any non-dict) → identical behavior to before this change, so existing callers/tests aren't broken.
  • When a name fails is_valid_toolset AND is found in known_plugin_toolsets[platform], emit a distinct, more actionable warning pointing at plugins.enabled and plugin installation, instead of the generic unknown-toolset message with its (here, irrelevant) hermes-<platform> guess.
  • Threaded through the one existing call site in hermes_cli/config.py (read_raw_config().get("known_plugin_toolsets")), reusing the same try/except-wrapped, best-effort validation pass that already runs after every config migration.
  • No new call sites, no new validation pass — purely widening the existing one.

Test plan

  • Extended tests/hermes_cli/test_toolset_validation.py with:
    • a disabled-plugin case producing the new plugin-specific warning (and confirming it does not also carry the typo-guess wording)
    • a case where the name is known for a different platform, confirming it still falls back to the generic unknown-toolset warning (the cross-check is platform-scoped)
    • omitted/malformed known_plugin_toolsets (None, [], a string, {} with a non-list value) all falling back to the pre-existing generic behavior
  • Ran scripts/run_tests.sh tests/hermes_cli/test_toolset_validation.py tests/hermes_cli/test_config.py — 153 tests, all passing, no regressions in the config-migration suite that exercises this call site.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard labels Jul 2, 2026
@dante32683

dante32683 commented Jul 2, 2026 •

Copy link
Copy Markdown
Author

FYI for whoever reviews this, the linked mirror hashbender#601 (auto-backlinked above) carries this same fix plus one unrelated hunk in hermes_cli/config.py adding a bots_require_inline_mention default to the Discord config, which isn't mentioned in its (otherwise identical) PR description and isn't part of this change. Flagging in case it's relevant during review.

@dante32683

Copy link
Copy Markdown
Author

Filed #59547 for this with more background on the disabled-plugin-vs-typo confusion, in case that's useful context for review.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused fix. The current implementation still has the reported gap: hermes_cli/toolset_validation.py:58-66 emits the generic warning for every unloaded toolset, while hermes_cli/tools_config.py:1838-1854 establishes that known_plugin_toolsets records the per-platform plugin snapshot needed to distinguish a disabled plugin.

Problems

  • The helper cases in tests/hermes_cli/test_toolset_validation.py cover the new branch, but there is no regression test for the changed production propagation at hermes_cli/config.py:5724 in this PR (current equivalent: hermes_cli/config.py:6153). A wiring regression there would silently restore the generic warning.

Suggested changes

  • Add a temp-HERMES_HOME migrate_config() test in tests/hermes_cli/test_config.py that supplies both maps and asserts results["warnings"] contains the plugin-specific warning.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
…ation

validate_platform_toolsets() (#38798) already warns when a
platform_toolsets entry is unknown, with a `hermes-<platform>` typo
guess. That guess is wrong for the common case where the name is a
real plugin toolset whose plugin is currently disabled or
uninstalled — the three toolset-tracking lists (plugins.enabled,
known_plugin_toolsets, platform_toolsets) can drift out of sync with
no cross-check between them.

known_plugin_toolsets (hermes_cli/tools_config.py's
_save_platform_tools) already records, per platform, which plugin
toolset keys were valid the last time `hermes tools` saved that
platform. Cross-referencing it lets validate_platform_toolsets tell
"this was a real plugin toolset, now unavailable" apart from "this
was never a valid name," and point at the actual likely cause
(plugins.enabled / installation) instead of a misleading typo
suggestion.

known_plugin_toolsets is a new optional third argument, threaded
through from config.py's existing validate_platform_toolsets call
site; omitting it preserves the prior generic-warning behavior.
Address review: the helper branches were covered but the production wiring
at hermes_cli/config.py was not, so dropping the known_plugin_toolsets
argument would silently restore the generic warning. Add temp-HERMES_HOME
migrate_config() tests asserting the plugin-specific warning when the map is
supplied, and the unknown-toolset warning when it is not.
@dante32683
dante32683 force-pushed the fix/toolset-cross-list-validation branch from da2542c to 0048883 Compare July 16, 2026 00:50
@dante32683

Copy link
Copy Markdown
Author

Thanks — addressed in 0048883 (rebased onto current main; the propagation now sits at hermes_cli/config.py:6165).

Added TestPlatformToolsetValidationWiring in tests/hermes_cli/test_config.py with a temp-HERMES_HOME migrate_config() test that supplies both maps and asserts results["warnings"] carries the plugin-specific warning (and not the "did you mean" guess), plus a companion asserting the generic unknown-toolset warning still fires when known_plugin_toolsets is absent — that's what distinguishes the two cases.

I verified the test actually guards the wiring: dropping the known_plugin_toolsets argument at the call site makes it fail, and restoring it passes. 178 pass across the two files.

@dante32683 dante32683 closed this Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants